-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to opt into full-RBF when sending funds #7132
Add option to opt into full-RBF when sending funds #7132
Conversation
@@ -405,6 +405,7 @@ std::string HelpMessage(HelpMessageMode mode) | |||
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET)); | |||
strUsage += HelpMessageOpt("-maxtxfee=<amt>", strprintf(_("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)"), | |||
CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE))); | |||
strUsage += HelpMessageOpt("-optintofullrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_OPT_INTO_FULL_RBF)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter names shouldn't imply a default value (as "opt-in" does).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps -walletusefullrbf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luke-jr What's your suggested name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-walletrbf maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, don't forget the #ifdef ENABLE_WALLET guard...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: name, other names like -spendzeroconfchange and -sendfreetransactions have similar grammar as -optintofullrbf, so I'm inclined to continue that pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just failed to expand more of the code visible here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for reminding me though - that it was under ENABLE_WALLET was just luck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, is it full RBF or opt-in RBF ?
We've been using fullRBF to refer to the relay policy that ignores the opt-in flag and always does RBF regardless of the sender's intentions.
Can you just call it optinRBF instead of optinfullRBF ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @luke-jr 's suggestion: -walletrbf
.
Wasn't this already closed? |
@dcousens The closed version defaulted to opt-in=true. |
utACK |
1 similar comment
utACK |
utACK, but does the wallet actually support the re-creation of a transaction in any sane way? Or is this a political thing? concept ACK only if the wallet actually allows you to re-broadcast a replacement. |
@dcousens I suppose it's useful for testing software-- e.g. if you want something that identifies these transactions you need to generate some, if nothing else. Actual replacement will be a non-trivial amount of work. Concept ACK at least... |
412bf57
to
623f29d
Compare
Then use
IMHO, then that is what should prefix this flag. Otherwise its just misleading. |
Real world use case: Core's wallet is used in day-to-day operation, but occasionally stuck transactions need replacing to get confirmed, which the user then uses an external script for. It's not pretty, but it's a real use case. If it was only needed for testing stuff, I'd say +1 to just telling people to use sendrawtransaction also - or modify the code. |
@dcousens I have scripts that do the rebroadcast, and using those scripts is a pain if Bitcoin Core isn't already sending txs with opt-in enabled, allowing those scripts to be used with existing txs. I'm sure we'll have even better support in the future, but this is a trivial first-step. Particularly with the opt-in defaulting to false, I can't see how this is political - we're just making it a little easier to use a feature that we've already merged. |
Sure, concept ACK then.
If you don't accept the above use case as a possibility, then, IMHO, it didn't really serve any other purpose. I didn't personally think people would be mixing scripts and the UI. |
623f29d
to
5fc02e2
Compare
Rebased |
5fc02e2
to
0292b63
Compare
Just changed this to set nSequence to maxint-2 instead, so that more of the nSequence bits are identical to non-opt-in behavior. This might come in handy if we, for instance, ever implement proof-of-stake blocksize voting and need a default option that matches what most wallets already do. |
re-ACK |
What do you mean by "100 nSequence bits"? On 2 December 2015 12:32:11 GMT+08:00, Daniel Cousens notifications@github.com wrote:
|
|
Sorry, I don't see what's the advantage of that; maxint-2 seems simpler. On 2 December 2015 12:38:06 GMT+08:00, Daniel Cousens notifications@github.com wrote:
|
@petertodd my understanding is you're expanding the non-opt-in space by 1 to allow for possible future usage. |
Oh! That's not at all what I'm doing! This is just the wallet code; I'm changing what txs the wallet produces, not changing the rules for what is considered RBF opt-in. On 2 December 2015 12:43:47 GMT+08:00, Daniel Cousens notifications@github.com wrote:
|
@petertodd my bad. Lost context. On that note then, why not |
@dcousens Because of #7132 (comment) Remember that all we need is for all users' to be using the same nSequence number for a given type of tx; for privacy the common standard is what matters, not exactly what number we pick. |
Sure. |
Concept ACK |
Tend to NACK. I think base work for this (starting with rawtx command) could be #7159. |
Agree with @jonasschnelli here, a global option is too coarse, need a way to have control over this per transaction. |
Why not both? On 25 April 2016 05:43:48 GMT-04:00, "Wladimir J. van der Laan" notifications@github.com wrote:
|
Especially as it's aimed at third-party scripts it is better if those scripts can specify the option, without requiring the user to change yet another option before they can be used. |
0292b63
to
630857a
Compare
Rebased |
needs rebase |
@arowser can you stop posting "Can one of the admins verify this patch?" in every pull, this is annoying and completely redundant. You can help by reviewing or testing the code. |
630857a
to
e272c26
Compare
Rebased |
Would this work with further nit: anything "optin" will become "optout" if the default changes, so perhaps: "sendfullrbf"? Makes it clearer it's about sending, not mempool replacement. concept ACK, I'd really like something for 0.13. |
BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins) | ||
txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(), | ||
std::numeric_limits<unsigned int>::max()-1)); | ||
std::numeric_limits<unsigned int>::max() - (fOptIntoFullRbf ? 2 : 1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe constants for 1 and 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those numbers aren't special, the 1st number just has to be bigger than 1. Not sure if that's a candidate for constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mqybe more documentation solves it. I just cannot look at that line without wondering what the heck 1 and 2 mean...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. The comment should be revised further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this comment:
"BIP125 defines opt-in RBF as any nSequence < maxint-1, so we use the highest possible value in that range (maxint-2) to avoid conflicting with other possible uses of nSequence, and in the spirit of "smallest posible change from prior behavior""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe we could introduce std::numeric_limits<unsigned int>::max()-1
as some constant somewhere. SEQUENCE_OPT_OUT = std::numeric_limits<unsigned int>::max()-1
is used in other places (mempool) as well.
And then comment here "Use any value less than SEQUENCE_OPT_OUT according to BIP125." or use the comment you wrote above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fOptIntoFullRbf ? SEQUENCE_OPT_OUT -1 : SEQUENCE_OPT_OUT
It's clrearer to me than:
std::numeric_limits<unsigned int>::max() - (fOptIntoFullRbf ? 2 : 1)
Specially with the ""Use any value less than SEQUENCE_OPT_OUT..." comment.
Is there anything blocking this? (besides needing rebase again, sorry) |
Concept ACK. Needs rebase. |
Closing in favor of #8601 |
Useful for anyone using third-party scripts to make use of RBF functionality.
Defaults to sending txs with full-RBF off.